Code Review — REQUEST CHANGES

Что сделано хорошо

• Чистая архитектура: HunkToolbarAction enum, InlineDiffProvider static methods, HunkToolbarView как отдельный AppKit view
• Dismiss из всех источников (Escape, click outside, data change) правильно централизован через onHunkDismissed callback
• Wrap-around навигация, stale hunk fallback — продуманные edge cases
• 37 тестов покрывают логику навигации, dismiss, summary, line range
• Refactoring dismissExpandedHunk() — устранило дублирование кода Escape + click outside

CRITICAL — masksToBounds + shadow конфликт

Pine/HunkToolbarView.swift, строки 60-89:

layer?.masksToBounds = true   // line 61
// ...
layer?.shadowRadius = 3       // line 88
layer?.shadowOpacity = 1      // line 89

masksToBounds = true обрезает shadow. Тень НИКОГДА не будет видна. Это либо мёртвый код (shadow), либо баг (нет тени у toolbar). Нужно убрать masksToBounds = true и вместо этого использовать layer?.cornerCurve = .continuous только для cornerRadius, или сделать два слоя (inner + outer).

CRITICAL — динамические цвета не обновляются при смене appearance

Pine/HunkToolbarView.swift, строки 63-82:

layer?.backgroundColor = bgColor.cgColor и layer?.borderColor = borderColor.cgColor — .cgColor вычисляется ОДИН РАЗ при init. При переключении Dark/Light mode цвета останутся от первоначальной темы. Нужен override viewDidChangeEffectiveAppearance() или updateLayer() для пересчёта layer?.backgroundColor и layer?.borderColor.

Аналогичная проблема у separator в makeSeparator() — layer?.backgroundColor не обновится.

IMPORTANT — O(n) линейный скан в lineNumberAtPoint на каждый mouseDown

Pine/CodeEditorView.swift, lineNumberAtPoint(_:), строки 540-543:

for i in 0..<min(charIndex, source.length) where source.character(at: i) == ASCII.newline {
    line += 1
}

Для файла в 100K строк это ~100K итераций на каждый клик мыши. В Coordinator уже есть lineStartsCache: LineStartsCache с O(log n) binary search. lineNumberAtPoint должен использовать его, а не сканировать текст заново. Проблема: lineStartsCache живёт в Coordinator, а метод — в GutterTextView. Нужно либо передать cache в GutterTextView, либо вынести логику в Coordinator.

IMPORTANT — дублирование line-to-charOffset кода

Pine/CodeEditorView.swift, строки 1952 и 2037:

Один и тот же паттерн "while loop считает newline" повторяется в positionToolbar и scrollToHunk. Плюс третий раз в lineNumberAtPoint. Нужно вынести в private helper method (или использовать lineStartsCache).

IMPORTANT — toolbar не зануляется при hideHunkToolbar

Pine/CodeEditorView.swift, hideHunkToolbar():

func hideHunkToolbar() {
    hunkToolbar?.isHidden = true
    hunkToolbar?.removeFromSuperview()
}

Toolbar удаляется из view hierarchy, но ссылка hunkToolbar сохраняется. При следующем showHunkToolbar переиспользуется старый экземпляр (if let existing = hunkToolbar). Это не memory leak, но при повторном показе toolbar будет addSubview ЗАНОВО. Это ок как design choice, но стоит добавить комментарий, что переиспользование — intentional. Либо занулить: hunkToolbar = nil в hideHunkToolbar.

IMPORTANT — accessibility IDs не в AccessibilityIdentifiers.swift

Toolbar accessibility IDs ("hunk-toolbar", "hunk-toolbar-summary", "hunk-toolbar-previous" и т.д.) захардкожены в HunkToolbarView и HunkToolbarAction, но не добавлены в Pine/AccessibilityIdentifiers.swift — нарушение проектного соглашения. Все accessibility identifiers должны быть в одном месте.

IMPORTANT — tooltip строки не локализованы

"Previous Change", "Next Change", "Restore", "Dismiss" захардкожены прямо в HunkToolbarView.swift. По conventions проекта строки должны идти в Strings.swift и Localizable.xcstrings.

SUGGESTION — тест hunkToolbarViewActionCallback не тестирует реальное нажатие кнопки

toolbar.onAction?(.restore)  // это ручной вызов callback, не simulate button click

Лучше бы через button.performClick(nil). Текущий тест проверяет, что Swift closures работают, а не что кнопки подключены к actions.

SUGGESTION — нет confirmation для Restore action

Restore необратимо откатывает изменения в hunk. Нет NSAlert/confirmation dialog. VS Code показывает confirmation для revert. Стоит хотя бы создать issue если не хотите делать сейчас.

Итого для merge:

1. Fix masksToBounds + shadow conflict
2. Fix dynamic color update on appearance change
3. Использовать lineStartsCache вместо O(n) scan в lineNumberAtPoint
4. Вынести дублированный line-to-charOffset код в helper
5. Перенести accessibility IDs в AccessibilityIdentifiers.swift
6. Локализовать строки в Strings.swift

Повторное ревью PR #691

Предыдущие замечания — статус

Все 6 замечаний из прошлого ревью исправлены:

1. masksToBounds — убран, shadow видна. Добавлен тест hunkToolbarViewShadowNotClipped.
2. viewDidChangeEffectiveAppearance — переопределён, вызывает updateAppearanceColors(). Фоны и бордеры обновляются.
3. O(log n) lineNumberAtPoint — использует lineStartsCache с binary search, fallback на O(n).
4. Единый charOffsetForLine — один static метод с cache parameter, 1-based нумерация.
5. Accessibility IDs — 6 ID в AccessibilityIdentifiers.swift.
6. Tooltip strings — 4 строки в Strings.swift + Localizable.xcstrings (en/ru).

Новые замечания

Important: Separator .cgColor не обновляется при смене темы

HunkToolbarView.makeSeparator() (строка ~164) устанавливает layer?.backgroundColor через .cgColor один раз при создании view:

sep.layer?.backgroundColor = NSColor.separatorColor.withAlphaComponent(0.3).cgColor

Этот .cgColor — снимок текущей темы. При переключении Dark/Light сепараторы останутся в старом цвете. updateAppearanceColors() обновляет фон и бордер основного view, но не трогает сепараторы.

Fix: Сохранить ссылки на separator views и обновлять их layer?.backgroundColor внутри updateAppearanceColors(), аналогично тому, как обновляются borderColor и backgroundColor.

Suggestion: Дублированные MARK-комментарии

В GutterTextView два почти идентичных MARK:

• Строка 515: // MARK: - Click outside hunk dismisses expanded hunk
• Строка 565: // MARK: - Click outside hunk dismisses expanded inline diff

mouseDown под первым, shouldDismissHunkOnClick / dismissExpandedHunk под вторым, а keyDown (Escape) между ними. Логически это одна секция "Hunk dismiss" — стоит объединить в один MARK и сгруппировать mouseDown, shouldDismissHunkOnClick, dismissExpandedHunk, keyDown рядом.

Code Review -- APPROVED (self-PR, cannot approve via API)

Separator views fix -- VERIFIED

Fix is correct: separatorViews stored as private(set), updated in updateAppearanceColors(), called from viewDidChangeEffectiveAppearance(). Test covers the scenario.

Minor issues (non-blocking)

1. Double hideHunkToolbar() calls in handleHunkToolbarAction: both .dismiss and .restore cases trigger onHunkDismissed callback (via setting expandedHunkID = nil) which calls hideHunkToolbar(), then explicitly call hideHunkToolbar() again. Idempotent so no bug, but redundant.
2. Header comment mismatch in HunkToolbarView.swift: says ← Prev | ↑ summary (2/5) ↓ | Restore | ✕ but actual order is Prev Next | summary | Restore Dismiss.

Positives

• Excellent test coverage (457 lines, edge cases, boundary conditions)
• Clean charOffsetForLine 0-based to 1-based migration
• Proper [weak self] in callbacks
• Correct xcstrings formatting
• Full accessibility identifiers